Skip to content

feat: array/list parameter support for Gestalt-DI annotations - #160

Merged
Cervator merged 4 commits into
developfrom
temp/terasology-gestalt-di-migration-changes
Jul 28, 2026
Merged

feat: array/list parameter support for Gestalt-DI annotations#160
Cervator merged 4 commits into
developfrom
temp/terasology-gestalt-di-migration-changes

Conversation

@BenjaminAmos

@BenjaminAmos BenjaminAmos commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

These changes are needed for Terasology to work with Gestalt DI. This is to go along with the main Terasology pull request.

It appears to do the following (although I am no longer certain of this):

  • Add additional input validation to catch null beans when closing the context
  • Upgrade the JavaPoet library (although I am unable to recall why by this point)
  • Fixes for using array/list values as annotation parameters when inspected by Gestalt DI's annotation processor
  • Change !semver.preReleaseVersion().isEmpty() to semver.preReleaseVersion().isPresent() (this may have been due to a breaking ABI change in the JSemver library)

@BenjaminAmos BenjaminAmos changed the title temp: Terasology Gestalt-DI migration changes feat: array/list parameter support for Gestalt-DI annotations Aug 10, 2025
@BenjaminAmos
BenjaminAmos force-pushed the temp/terasology-gestalt-di-migration-changes branch from 2d99dbb to b2a6b05 Compare March 25, 2026 11:28
@Cervator

Copy link
Copy Markdown
Member

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Mar 31, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@coderabbitai

coderabbitai Bot commented Mar 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 750dacec-625c-4eab-be23-3db3181da06d

📥 Commits

Reviewing files that changed from the base of the PR and between 3626728 and c46fd01.

📒 Files selected for processing (1)
  • gradle.properties

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Prevented recursive closure of the dependency-injection context.
    • Improved annotation processing to format values using the annotation method’s declared return type, including better enum and list handling.
    • Corrected classpath resource path resolution by properly distinguishing directory vs. file lookups.
    • Fixed snapshot detection to use correct pre-release presence semantics.
  • Chores
    • Updated the JavaPoet library to a newer patch version.
    • Bumped the project version to 8.0.2-SNAPSHOT.

Walkthrough

Cross-module changes prevent self-closing bean contexts, update JavaPoet, improve annotation-value formatting, fix classpath path construction, correct snapshot detection, and bump the project version.

Changes

Core behavior and release updates

Layer / File(s) Summary
Bean context cleanup
gestalt-di/.../DefaultBeanContext.java
Avoids invoking close() on the context instance itself.
Annotation processing and dependency update
gestalt-inject-java/..., gestalt-inject-java/build.gradle.kts
Adds type-aware formatting for annotation lists and enums, and updates JavaPoet to 1.13.0.
Classpath resource handling
gestalt-module/.../ClasspathFileSource.java
Distinguishes file and directory lookups when constructing resource paths.
Version logic and project metadata
gestalt-module/.../Version.java, gradle.properties
Uses pre-release presence for snapshot detection and changes the project version to 8.0.2-SNAPSHOT.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

A rabbit hops through paths and types,
While safer closing code takes flight.
JavaPoet joins the springtime tune,
Snapshots bloom beneath the moon.
Eight-point-oh-two goes live—
Hoppy changes, all five alive! 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: array/list annotation parameter support in Gestalt-DI.
Description check ✅ Passed The description is clearly related to the changeset and mentions the same fixes and dependency updates.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch temp/terasology-gestalt-di-migration-changes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java`:
- Line 247: In the values.add(...) call that currently passes
executableElement.asType() into getValue(...), replace asType() with
executableElement.getReturnType() so getValue() receives the annotation member's
return type (not the full executable signature); update the invocation near
values.add(CodeBlock.of("$S,$L", executableElement.getSimpleName(),
getValue(executableElement.asType(), value.getValue()))) to use getReturnType()
to fix enum detection and correct empty-array literal generation (consistent
with the earlier usage at line 238).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8c5540a6-e2c1-414e-a2da-b99e7eb25ec1

📥 Commits

Reviewing files that changed from the base of the PR and between 33d0591 and b2a6b05.

📒 Files selected for processing (5)
  • gestalt-di/src/main/java/org/terasology/gestalt/di/DefaultBeanContext.java
  • gestalt-inject-java/build.gradle.kts
  • gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java
  • gestalt-module/src/main/java/org/terasology/gestalt/module/resources/ClasspathFileSource.java
  • gestalt-module/src/main/java/org/terasology/gestalt/naming/Version.java

To help clarify the newest snapshot

@Cervator Cervator left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already been using a built snapshot of this for a while, just catching up with build logistics - bumped this to 8.0.2 for a clean snapshot, noted one test failure that may be inconsistent. Opting to merge to match reality then fix-forward the test as part of another newer PR also hitting test issues.

@Cervator
Cervator merged commit b204b06 into develop Jul 28, 2026
3 of 6 checks passed
@Cervator
Cervator deleted the temp/terasology-gestalt-di-migration-changes branch July 28, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants